-
Notifications
You must be signed in to change notification settings - Fork 456
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
UI tab backup #319
UI tab backup #319
Conversation
Is there a way to run the linter locally? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
- Added IPC handlers for managing editor flex center settings and open tabs (
electron/main/electron-store/ipcHandlers.ts
) - Introduced new
Tab
type and updatedStoreSchema
andStoreKeys
enums for tab management (electron/main/electron-store/storeConfig.ts
) - Added
EmptyPage
component for when no file is selected (src/components/EmptyPage.tsx
) - Introduced
TabProvider
for managing tabs within the application (src/components/Providers/TabProvider.tsx
) - Updated various components to include
widthType
prop for dynamic modal sizing (src/components/Common/Modal.tsx
,src/components/File/NewDirectory.tsx
,src/components/File/NewNote.tsx
,src/components/File/RenameDirectory.tsx
,src/components/File/RenameNote.tsx
,src/components/Flashcard/FlashcardMenuModal.tsx
,src/components/Flashcard/FlashcardReviewModal.tsx
)
29 file(s) reviewed, 13 comment(s)
Edit PR Review Bot Settings
src/components/Common/Modal.tsx
Outdated
@@ -6,17 +6,66 @@ interface ModalProps { | |||
children: React.ReactNode | |||
hideCloseButton?: boolean | |||
tailwindStylesOnBackground?: string | |||
widthType: number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: Consider using a string type for widthType
instead of number
for better type safety.
src/components/Common/Modal.tsx
Outdated
@@ -40,7 +89,7 @@ const ReorModal: React.FC<ModalProps> = ({ | |||
> | |||
<div | |||
ref={modalRef} | |||
className="flex max-w-4xl flex-col items-center justify-center rounded-lg border border-solid border-gray-600 bg-dark-gray-c-three shadow-xl" | |||
className={`bg-dark-gray-c-three rounded-lg shadow-xl w-${widthClass} max-w-6xl flex flex-col justify-center items-center border-solid border-gray-600 border`} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic: Using template literals for class names can lead to unexpected results if widthClass
is not a valid Tailwind class.
const deltaWidth = resizeSide === "left" ? -e.movementX : e.movementX; | ||
setWidth((prevWidth) => { | ||
const newWidth = prevWidth + deltaWidth; | ||
if (newWidth > setMaximumWidthCap) return setMaximumWidthCap; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic: Ensure the new width does not exceed the maximum width cap.
setWidth((prevWidth) => { | ||
const newWidth = prevWidth + deltaWidth; | ||
if (newWidth > setMaximumWidthCap) return setMaximumWidthCap; | ||
if (newWidth < 50) return 50; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic: Ensure the new width does not fall below the minimum width of 50.
/> | ||
<div | ||
className={`${ | ||
editorFlex ? "flex justify-center items-center h-screen p-4" : "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic: Potential issue: h-screen
class might cause layout issues if the editor content exceeds the viewport height.
/> | ||
<div | ||
className={`${ | ||
editorFlex ? "flex justify-center items-center h-screen p-4" : "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: Consider removing h-screen
to avoid potential overflow issues.
src/components/EmptyPage.tsx
Outdated
const EmptyPage = ({ vaultDirectory }) => { | ||
const handleCreateFile = () => { | ||
window.electronUtils.showCreateFileModal(vaultDirectory); | ||
console.log("Attemtping to create new file!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling: Typo in 'Attemtping'.
console.log("Attemtping to create new file!"); | |
console.log("Attempting to create new file!"); |
src/components/EmptyPage.tsx
Outdated
}; | ||
|
||
const handleCreateFolder = () => { | ||
window.electronUtils.showCreateDirectoryModal(vaultDirectory); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: Consider adding error handling for showCreateDirectoryModal
.
const syncTabsWithBackend = async (action, args) => { | ||
await window.electronStore.setCurrentOpenFiles(action, args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: Consider adding error handling for the async function to manage potential failures.
Cleaned up the code. Had to make a change in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
(updates since last review)
The pull request introduces several UI enhancements and tab management functionalities, along with various code refactorings and stylistic adjustments.
- Tab Management: Added handlers for managing tabs in
electron/main/electron-store/ipcHandlers.ts
and updatedTabProvider
insrc/components/Providers/TabProvider.tsx
. - UI Enhancements: Added padding to
File List
andTextEditor
content, and introducedEmptyPage
for when no file is opened (src/components/EmptyPage.tsx
). - Settings Reorganization: Moved
General Page
to the top of the settings and updatedGeneralSettings.tsx
for better readability. - Spell Check: Integrated
nspell
library for spell-checking (src/utils/strings.ts
) and updated related IPC handlers inelectron/preload/index.ts
. - Stylistic Changes: Removed semicolons and adjusted import orders for consistency across multiple files (
electron/main/electron-store/storeConfig.ts
,electron/main/electron-utils/ipcHandlers.ts
).
No significant security issues were introduced, but thorough testing is recommended to ensure the new functionalities work as expected.
37 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings
const removeTab = ({ tabId, idx, newIndex }: { tabId: string; idx: number; newIndex: number }) => { | ||
openTabs[idx].lastAccessed = false | ||
openTabs[newIndex].lastAccessed = true | ||
const updatedTabs = openTabs.filter((tab) => tab.id !== tabId) | ||
store.set(StoreKeys.OpenTabs, updatedTabs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic: Potential issue: Ensure idx
and newIndex
are valid indices to avoid runtime errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add the checking the bot recommends here @milaiwi ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'll go ahead and add it, but either way I'm pretty sure it is not possible for idx
and newIndex
not to be valid since tabId is guaranteed to exist which means idx
and newIndex
must exist.
@@ -40,7 +83,7 @@ const ReorModal: React.FC<ModalProps> = ({ | |||
> | |||
<div | |||
ref={modalRef} | |||
className="flex max-w-4xl flex-col items-center justify-center rounded-lg border border-solid border-gray-600 bg-dark-gray-c-three shadow-xl" | |||
className={`w-${widthClass} flex max-w-6xl flex-col items-center justify-center rounded-lg border border-solid border-gray-600 bg-dark-gray-c-three shadow-xl`} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic: Using template literals for class names can lead to unexpected results if widthClass
is not a valid Tailwind class.
openTabs.forEach((tab: Tab) => { | ||
if (tab.lastAccessed) { | ||
setOpenedLastAccess(true) | ||
openFileAndOpenEditor(tab.filePath) | ||
} | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic: Potential issue: openFileAndOpenEditor
might be called multiple times if multiple tabs have lastAccessed
set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with bot. Perhaps we should break out of the loop as soon as we open a file based on tab.lastAccessed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if you can break out of a forEach
since it is just an underlying recursive function. I'll change it to use .some
instead.
@samlhuillier That should be the final commit. Take a look at this just cleaned up sidebars and UI on file sidebar. LMK what you think! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
(updates since last review)
The pull request introduces several UI enhancements, tab management functionalities, and code refactorings.
- ResizableComponent.tsx: Removed
onResize
prop and max width cap logic, potentially introducing width management issues. - TitleBar.tsx: Removed
sidebarWidth
prop and added logic for handling the last accessed tab, enhancing usability. - EditorManager.tsx: Added opacity style, modified padding, and scrollbar visibility, potentially causing layout issues with
h-screen
. - MainPage.tsx: Removed
resizableWidth
state and addedoverflow-x-hidden
class, simplifying state management and preventing horizontal overflow. - global.css: Set scrollbar width to 0px, improving aesthetics but potentially affecting usability.
5 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
(updates since last review)
The pull request introduces UI enhancements, including tab support, padding adjustments, and layout improvements.
- src/components/Editor/EditorManager.tsx: Added overflow handling to
EditorContent
for better scrollability and layout management. - src/components/Editor/EditorManager.tsx: Adjusted padding for
TextEditor
content, configurable via settings. - Settings: Moved General Page to the top for better accessibility.
- UI: Introduced
EmptyPage
for scenarios with no open files. - Styling: Updated coloring and padding for improved aesthetics and usability.
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
Hey @milaiwi apologies for the delay. I really like the tabs experience! Super wicked piece of work! I'll start reviewing it and edit this comment re changes and suggestions.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
(updates since last review)
The pull request introduces UI enhancements, including tab support, padding adjustments, and layout improvements.
- scripts/notarize.js: Added a console log statement to indicate the start of the notarization process.
- src/components/MainPage.tsx: Integrated
EmptyPage
component for scenarios with no open files. - src/components/Settings/Settings.tsx: Moved General Page to the top for better accessibility.
- src/components/Editor/EditorManager.tsx: Adjusted padding for
TextEditor
content, configurable via settings. - src/components/Sidebars/IconsSidebar.tsx: Updated sidebar icons and added new modal triggers for better user interaction.
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
This reverts commit 3402938.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
(updates since last review)
The pull request introduces several UI enhancements and improvements to the notarization process.
- scripts/notarize.js: Enhanced logging and error handling, including directory tree printing for better debugging.
- src/components/MainPage.tsx: Integrated
EmptyPage
component for scenarios with no open files. - src/components/Settings/Settings.tsx: Moved General Page to the top for better accessibility.
- src/components/Editor/EditorManager.tsx: Added padding for
TextEditor
content, configurable via settings. - src/components/Sidebars/IconsSidebar.tsx: Updated sidebar icons and added new modal triggers for better user interaction.
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
(updates since last review)
The pull request introduces several UI enhancements and improvements to the notarization process.
- .prettierrc: Updated
endOfLine
tolf
for consistent line endings across OS. - package.json: Renamed
postbuild
script tonotarize
for clarity. - scripts/notarize.js: Added logging for better feedback during notarization.
- src/components/MainPage.tsx: Integrated
EmptyPage
component for scenarios with no open files. - src/components/Settings/Settings.tsx: Moved General Page to the top for better accessibility.
3 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
Yes run the linter like this:
making sure you've done an |
// Note: Do not put dependency on addTab or else removeTab does not work properly. | ||
// Typically you would define addTab inside the useEffect and then call it but since | ||
// we are using it inside a useContext we can remove it | ||
/* eslint-disable */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we not do something like:
const memoizedAddTab = useCallback((filePath) => {
addTab(filePath);
}, [addTab]);
useEffect(() => {
if (!currentFilePath) return;
memoizedAddTab(currentFilePath);
}, [currentFilePath, memoizedAddTab]);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addTab
already uses useCallback
, so this would essentially be the same thing. I'll take a deeper look on how to fix this
public/dictionaries/en_US/en_US.aff
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the reason for adding this? Until now, we've kept spellcheck fairly simple by just leveraging the user system spellcheck. Perhaps we should add this for every language if you think it's a value add...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's needed for replaceWord
. It takes the word that the spellcheck highlights as wrong and finds the correct word it should be. I don't think tiptap
has a built-in replaceWord
so we have to make a custom one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there no library for this either? Because I think having these big dictionary files is not something we should have in the codebase nor something we should maintain...
newEmbeddingModel: '[500px]', | ||
localLLMSetting: '[500px]', | ||
remoteLLMSetting: '[500px]', | ||
indexingProgress: '[850px]', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This stuff with mapping modal types to a width was removed a few commits back in favour of just allowing the child component to dictate the width, which I think leads to nice clean code 😅
Do you have strong opinions on needing to add this back? Can't you just set the width in the child component?
@@ -23,7 +23,7 @@ const ResizableComponent: React.FC<ResizableComponentProps> = ({ | |||
(e: MouseEvent) => { | |||
if (isDragging) { | |||
const deltaWidth = resizeSide === 'left' ? -e.movementX : e.movementX | |||
setWidth((prevWidth) => prevWidth + deltaWidth) | |||
setWidth(deltaWidth) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you add this? It breaks dragging of the sidebars...
Screen.Recording.2024-07-29.at.09.29.32.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally I made the tabs move depending on where the sidebar was located, but ended up scrapping that and just keeping it at a fixed location on the top left corner. I guess I didn't clean it up good when I scrapped. It's fixed now.
@@ -114,6 +115,13 @@ export const registerStoreHandlers = (store: Store<StoreSchema>, windowsManager: | |||
|
|||
ipcMain.handle('get-sb-compact', () => store.get(StoreKeys.IsSBCompact)) | |||
|
|||
ipcMain.handle('get-editor-flex-center', () => store.get(StoreKeys.EditorFlexCenter)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain what this feature does? I tried it in settings and didn't notice any changes in the editor nor anywhere else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: I now see a slight difference in padding - do you have strong opinions on adding this feature? I think it wouldn't really be clear to the user what "Editor Flex" is + I think we should just have a standard default for things like padding so that we reduce user load in having to make decisions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The difference in padding depends on your screen size. On a Mac or small monitor, the difference is negligible. However, on a standard monitor (21–24 inches), like the one I use, it significantly impacts the UI. I can try to change the name and subcaption to make it more clearer.
I think we should just have a standard default for things like padding so that we reduce user load in having to make decisions
I don't think this impacts user load that much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok sounds good. Why don't we even say something in the subcaption like "this is useful if you have a big monitor"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or perhaps there's even some kind of standard tailwind/css config for this kind of thing where you adjust padding based on screen size?
@@ -185,7 +185,7 @@ const useFileByFilepath = () => { | |||
editor.setOptions({ | |||
editorProps: { | |||
attributes: { | |||
spellcheck: spellCheckEnabled, | |||
spellcheck: spellCheckEnabled.toString(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change? I think this is what is causing spellcheck to not work on my machine...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally, the way spellCheck was stored and handled in the IPC was by maintaining and storing a state that had two values: "False" or "True". On the slider we converted it to a boolean, but stored it as a string. I thought this was weird so I changed it to be a boolean and converted it to a string when needed (in the case highlighted).
} | ||
|
||
const CreateAppearanceSection: React.FC = () => { | ||
export const CreateAppearanceSection = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this called CreateAppearanceSection
? It should be called AppearanceSection
given that it's a component not a function. Same with CreateEditorSection
TextGenerationTab = 'textGeneration', | ||
// RAG = "RAG", | ||
ANALYTICSTab = 'analytics', | ||
ChunkSizeTab = 'chunkSize', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why introduce new tabs that don't exist? Also nit but why is ANALYTICSTab
all capitalised?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why introduce new tabs that don't exist?
What do you mean?
Also nit but why is ANALYTICSTab all capitalised?
Fixed
@@ -116,25 +130,29 @@ const IconsSidebar: React.FC<IconsSidebarProps> = ({ openRelativePath, sidebarSh | |||
onClick={() => setIsNewNoteModalOpen(true)} | |||
> | |||
<div className="flex size-4/5 items-center justify-center rounded hover:bg-neutral-700"> | |||
<VscNewFile className="text-gray-200" size={22} title="New Note" /> | |||
<IconContext.Provider value={newFileContextValue}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these IconContext.Provider
s necessary? They seem to introduce a lot of baggage code when we could just pass the colour in directly no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think you're right here. I ended up replacing all IconContext.Providers to just color with the appropriate ternaries.
id="titleBarFileNavigator" | ||
className="ml-1 flex h-[28px] cursor-pointer items-center justify-center px-2 text-white hover:rounded-md hover:bg-dark-gray-c-three" | ||
onClick={() => { | ||
window.electronUtils.showCreateFileModal('') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again as above - this functionality should be passed in either as a prop or a provider so that we avoid unnecessary extra calls to the backend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll consider the provider, but I disagree with the prop suggestion. In reality, this won't be called frequently enough to worry about backend costs. However, making it a prop would make the code messier and less readable.
@@ -44,13 +44,18 @@ export const registerLLMSessionHandlers = (store: Store<StoreSchema>) => { | |||
event.sender.send('anthropicTokenStream', chatHistory.id, chunk) | |||
} | |||
|
|||
const transformedChatHistory: ChatCompletionMessageParam[] = chatHistory.displayableChatHistory.map((message) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the reason for this change out of interest?
Something to note is that we are going to move the LLM calling logic all to the renderer process, it'll make it so much easier to not have to deal with all this messy logic with chunks being passed via IPC 😅
I'll take a look at this again tonight and answer the questions |
Closing this and opening a new one. @samlhuillier Check #339 |
Added
Let me know if any bugs